cmd/crossplane/xrd/generate.go (1)

53-53: ⚡ Quick win

Clarify what --path is relative to

Could we keep a bit more context in the help text here?
On Line 53, Output path. is pretty terse and doesn’t tell users this is within the project APIs layout.

Suggested wording

-	Path        string `help:"Output path."                    optional:""`
+	Path        string `help:"Output path within the project's APIs directory." optional:""`

As per coding guidelines: "**/cmd/**: Review CLI commands for proper flag handling, help text, and error messages. Ensure commands follow Crossplane CLI conventions."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/crossplane/xrd/generate.go` at line 53, The help text for the Path flag
(struct field Path in generate.go) is too terse; update its `help` string to
clearly state that the provided output path is relative to the project root and
should point into the APIs layout (e.g., under the project's api/ or apis/
directory), e.g. "Output path relative to project root (destination within
project APIs layout, e.g. api/ or apis/)." Ensure you only change the
`help:"..."` value for the Path field.

cmd/crossplane/function/help/generate.md (1)

2-3: 💤 Low value

Consider whether "idempotently" is clear for all users.

The term "idempotently" is technically precise but may not be familiar to all CLI users. Since this help text falls under cmd/**, the coding guidelines emphasize user-friendliness and avoiding internal technical details.

Would simpler phrasing like "adds the new function to the end of a Composition's pipeline (if not already present)" or "safely adds the new function" be clearer while still conveying the behavior?

That said, if "idempotently" is intentional and aligns with Crossplane terminology conventions, it's fine to keep—just wanted to check!

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/crossplane/function/help/generate.md` around lines 2 - 3, The help text
uses the term "idempotently" which may be unclear to some users; replace that
word in the generate help text where it says "idempotently adds the new function
to end of a Composition's pipeline" with clearer wording such as "adds the new
function to the end of a Composition's pipeline (if not already present)" or
"safely adds the new function to the end of a Composition's pipeline" so the
behavior is explicit and user-friendly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

.github/workflows/ci.yml (1)

203-206: ⚡ Quick win

Consider adding checksum validation for the Vale download.

The workflow downloads and executes a Vale binary without verifying its integrity. While the risk is relatively low with a pinned version over HTTPS, adding SHA256 checksum validation would follow security best practices and protect against potential supply-chain attacks or MITM scenarios.

🔐 Proposed enhancement

You can add checksum validation like this:

       - name: Install Vale
         env:
           VALE_VERSION: "3.14.2"
+          VALE_SHA256: "a1b2c3d4e5f6..." # Replace with actual checksum from release page
         run: |
-          wget https://github.com/errata-ai/vale/releases/download/v${{env.VALE_VERSION}}/vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz && tar -xvzf vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz
+          wget https://github.com/errata-ai/vale/releases/download/v${{env.VALE_VERSION}}/vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz
+          echo "${{env.VALE_SHA256}}  vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz" | sha256sum -c
+          tar -xvzf vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz

The checksum can be found on the Vale releases page.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ci.yml around lines 203 - 206, The "Install Vale" workflow
step currently downloads and extracts the Vale tarball without verifying
integrity; add a SHA256 checksum validation using the VALE_VERSION variable
before extracting: download or define the expected checksum for
vale_${{env.VALE_VERSION}}_Linux_64-bit.tar.gz, verify the downloaded file with
sha256sum (or similar) and fail the job on mismatch, then only tar -xvzf after
successful verification; update the "Install Vale" step to perform the checksum
fetch/verification and ensure the step exits non-zero on verification failure.

cmd/crossplane/docs.go (1)

131-133: ⚡ Quick win

Consider trimming whitespace from individual novale rules.

If a developer writes novale:"rule1, rule2" (with a space after the comma), the split will preserve that leading space in "rule2". This could cause the generated vale comment to have extra whitespace.

🧹 Proposed enhancement

 if n.Tag != nil && n.Tag.Get("novale") != "" {
-	dc.NoVale = strings.Split(n.Tag.Get("novale"), ",")
+	parts := strings.Split(n.Tag.Get("novale"), ",")
+	for i := range parts {
+		parts[i] = strings.TrimSpace(parts[i])
+	}
+	dc.NoVale = parts
 }

Or more concisely using a helper:

 if n.Tag != nil && n.Tag.Get("novale") != "" {
-	dc.NoVale = strings.Split(n.Tag.Get("novale"), ",")
+	dc.NoVale = slices.Map(strings.Split(n.Tag.Get("novale"), ","), strings.TrimSpace)
 }

(Note: slices.Map requires Go 1.24+; if not available, use the loop version)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/crossplane/docs.go` around lines 131 - 133, The current assignment to
dc.NoVale uses strings.Split(n.Tag.Get("novale"), ",") which preserves any
surrounding whitespace; update the logic in cmd/crossplane/docs.go where
n.Tag.Get("novale") is read so each entry is trimmed (e.g., map each element
with strings.TrimSpace or use slices.Map + strings.TrimSpace) before assigning
to dc.NoVale; ensure empty strings are preserved/handled consistently as the
original behavior requires.

cmd/crossplane/trace/trace.go (1)

80-80: 💤 Low value

Capitalization consistency check.

The help text for Concurrency is "load concurrency" (lowercase, no period), while all other flag help texts in this struct start with a capital letter and end with a period. Was the lowercase intentional (perhaps to address a specific Vale rule), or would "Load concurrency." be more consistent with the rest of the help text?

Thanks for improving the help text clarity throughout this command! 🎉

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/crossplane/trace/trace.go` at line 80, The help text for the Concurrency
flag is inconsistent with other flags (it reads "load concurrency" lowercase and
no period); update the struct tag on Concurrency to use sentence-style
capitalization and punctuation (e.g., "Load concurrency.") so it matches the
rest of the flags' help texts; locate the Concurrency field in the trace.go
struct and change the help tag value accordingly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes